feat(authz): add step 1 for the role assignment wizard#109
Conversation
|
Thanks for the pull request, @bra-i-am! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
b1d55c0 to
e2a71c0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
==========================================
+ Coverage 95.88% 96.13% +0.24%
==========================================
Files 70 76 +6
Lines 1602 1783 +181
Branches 337 385 +48
==========================================
+ Hits 1536 1714 +178
- Misses 64 67 +3
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3b13e69 to
5c518c8
Compare
6e294c5 to
d5990f0
Compare
eddffbd to
15be337
Compare
319a15a to
bf1004c
Compare
4ac83a8 to
49aeb2c
Compare
|
What is the PR (or PRs) that introduces the required endpoints? Has it already merged? |
|
@arbrandes, sorry for not including it in the cover letter earlier. The PR is openedx/openedx-authz#245 and has already been merged. |
| onChange={setUsers} | ||
| invalidUsers={invalidUsers} | ||
| placeholder={intl.formatMessage(messages['wizard.step1.users.placeholder'])} | ||
| hasNetworkError={!!validationError || invalidUsers.length > 0} |
There was a problem hiding this comment.
I'm not a big fan of this presentation of the error in terms of the service availability (network errors), because can be confused as a validation error.
I think is better having a separate component for it, maybe a alert on top or maybe the toast message (if still relevant for the redesign)
I would like to have others option including @maguilarUXUI
There was a problem hiding this comment.
Yes, and the validation error is presented as is showing in the proposal design. I'm not discussing that.
What I am talking about is error 500, the validation service is not available. I think those ones should be presented in another way, such as Alert on top instead of the red border and red message at the bottom.
There was a problem hiding this comment.
In this case I think the best option is a toast with the retry option if is possible.
There was a problem hiding this comment.
e6a5b2c to
12c25ea
Compare
| } | ||
| } catch (error) { | ||
| setValidationError(intl.formatMessage(messages['wizard.validate.error'])); | ||
| showErrorToast(error, validateUsersAndProceed); |
There was a problem hiding this comment.
is this correct? here you are passing the same function as a retry handler
There was a problem hiding this comment.
@jesusbalderramawgu, yees, that's the expected behavior when there is an unexpected error (e.g. 5xx)
| const handleUsersChange = useCallback((value: string) => { | ||
| setInvalidUsers((prev) => (prev.length > 0 ? [] : prev)); | ||
| setValidationError(null); | ||
| setUsers(value); |
There was a problem hiding this comment.
not sure but I think you should add here a setValidatedUsers([])
because validateUsers becomes stale if user edits the input after validation
There was a problem hiding this comment.
this is already covered; the only way to reach step 2 (where Save is available and a stale validatedUsers really matters) is through validateUsersAndProceed, which always overwrites validatedUsers with the fresh list. Do you think it is better to change it?
| const isError = invalidUsers.length > 0 || !!validationError; | ||
|
|
||
| useEffect(() => { | ||
| if (isError && usersInputRef.current) { |
There was a problem hiding this comment.
this will trigger on any error change even unrelates one, don't you need to refine it a little bit?
something like
if ((invalidUsers.length > 0 || validationError) && usersInputRef.current)
| } | ||
| }, [isError]); | ||
|
|
||
| const canProceed = !!users.trim() && !!selectedRole; |
There was a problem hiding this comment.
here if we have a string "a,,," it passes right?
maybe you could do something like
const parsed = parseUsers(users);
const canProceed = parsed.length > 0 && !!selectedRole;
| onClose(); | ||
| }; | ||
|
|
||
| const parseUsers = (input: string): string[] => input |
There was a problem hiding this comment.
this is recreated on every render, I don't see any dependency so you could jus hoist it outside
| )} | ||
|
|
||
| {/* Actual textarea — text is transparent when overlay is active */} | ||
| <textarea |
There was a problem hiding this comment.
missing ARIA attributes
| )} | ||
|
|
||
| {/* Actual textarea — text is transparent when overlay is active */} | ||
| <textarea |
There was a problem hiding this comment.
also, wouldn't be better to use the textarea from paragon? instead of this?
There was a problem hiding this comment.
not here... this component deliberately uses a raw <textarea> because it needs pixel-perfect alignment between the textarea and the overlay div for the individual usernames red highlighting. Paragon's Form.Control wraps the textarea in extra markup and applies its own styles, which would break that alignment :(
| } from 'react'; | ||
|
|
||
| // Shared styles between overlay and textarea to keep text positions in sync | ||
| const INPUT_STYLE: React.CSSProperties = { |
There was a problem hiding this comment.
for styling you should add paragon classes or if you need custom css, you should add it to the scss files
| ...INPUT_STYLE, | ||
| position: 'absolute', | ||
| inset: 0, | ||
| background: '#fff', |
There was a problem hiding this comment.
please check my comment above about styles
| course: intl.formatMessage(messages['wizard.step1.roles.contextLabel.course']), | ||
| }; | ||
|
|
||
| const rolesByContext = roles.reduce<Record<string, RoleMetadata[]>>((acc, role) => { |
There was a problem hiding this comment.
this has a performance issue, it runs on every render.
you could wrap it in useMemo using roles as dependency
| onChange={(e) => setSelectedRole(e.target.value)} | ||
| className="pl-4" | ||
| > | ||
| {rolesByContext[context].map((role) => { |
There was a problem hiding this comment.
I suggest this to be safer
(rolesByContext[context] || []).map()
|
|
||
| const orderedContexts = CONTEXT_ORDER.filter((ctx) => rolesByContext[ctx]); | ||
|
|
||
| const adminUrl = `${getConfig().LMS_BASE_URL}/admin`; |
There was a problem hiding this comment.
you could move this outside the component
| <div className="select-users-and-role-step"> | ||
| {/* Users Section */} | ||
| <h3 className="text-primary mb-2">{intl.formatMessage(messages['wizard.step1.users.heading'])}</h3> | ||
| <Form.Group controlId="users-input"> |
There was a problem hiding this comment.
the form is missing ARIA attributes
rodmgwgu
left a comment
There was a problem hiding this comment.
Tested in my local and functionality looks good, I'll approve once Jesus' comments are addressed. Thanks!
| defaultMessage: 'Retry', | ||
| description: 'Label for retry button.', | ||
| }, | ||
| 'library.authz.manage.assign.role.wizard.button': { |
There was a problem hiding this comment.
I do not see where this message is being used.
| }); | ||
| }, []); | ||
|
|
||
| // TODO: replace with real assignment API call using validatedUsers, selectedRole, selectedScopes |
There was a problem hiding this comment.
Is this TODO still pending?
There was a problem hiding this comment.
yep, this handleSave is addressed in the Step 2 PR
There was a problem hiding this comment.
I would suggest a more descriptive name for this folder, like role-assignation-wizard or assign-role-wizard, role-assignation.
There was a problem hiding this comment.
And try to keep the main component files on the root of this folder and for the rest of the components create a /components folder
| const [searchParams] = useSearchParams(); | ||
| const initialUsers = searchParams.get('users') || ''; | ||
| const raw = searchParams.get('from') ?? ''; | ||
| const returnTo = raw.startsWith('/') ? raw : ROUTES.HOME_PATH; |
There was a problem hiding this comment.
Suggested by Claude: In AssignRoleWizardPage.tsx, there's a check raw.startsWith('/') to prevent external URLs. However, //evil.com also starts with / and would be treated as a protocol-relative URL if ever used in an or
window.location. While navigate() from react-router likely handles this safely, the validation should be stricter:
// Current
const returnTo = raw.startsWith('/') ? raw : ROUTES.HOME_PATH;
// Safer — reject protocol-relative URLs
const returnTo = (raw.startsWith('/') && !raw.startsWith('//')) ? raw : ROUTES.HOME_PATH;
The test covers https://evil.com but not //evil.com.
| .map((u) => u.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| const validateUsersAndProceed = async () => { |
There was a problem hiding this comment.
Perhaps we should guard here and check for validateUsersMutation.isPending, to avoid double invocation which can happen when the user clicks quickly, which could happen even if we are disabling the button (there is a delay while React re-renders the button as disabled), or when this is called on the retry toast.
| @@ -26,10 +26,18 @@ export const CONTENT_LIBRARY_PERMISSIONS = { | |||
| // Note: this information will eventually come from the backend API | |||
| // but for the MVP we decided to manage it in the frontend | |||
| export const libraryRolesMetadata: RoleMetadata[] = [ | |||
There was a problem hiding this comment.
This is also defined in src/authz-module/constants.ts, I think we should keep just one.
|
|
||
| export const DEFAULT_FILTER_PAGE_SIZE = 5; | ||
| // Course Permission Keys | ||
| export const COURSE_PERMISSIONS = { |
There was a problem hiding this comment.
I don't see this being used in this PR, can we remove?
|
@rodmgwgu, @jesusbalderramawgu, @jacobo-dominguez-wgu thank you for your help reviewing this — I’ve addressed the feedback you shared. Please let me know if there’s anything else that should be resolved. I’ll let the tests run, and I’ll take care of any issues tomorrow morning if anything comes up. |
d7d37a7 to
ed75ec9
Compare
jacobo-dominguez-wgu
left a comment
There was a problem hiding this comment.
Thank you so much! I have tested it with manual valid and invalid usernames and emails input and with a preset user everything seems to be working well.
|
Lets wait til tomorrow for @rodmgwgu´s comments before merging. |
| const intl = useIntl(); | ||
| const { showErrorToast } = useToastManager(); | ||
| const [activeStep, setActiveStep] = useState<StepKey>(STEPS.SELECT_USERS_AND_ROLE); | ||
| const [users, setUsers] = useState(initialUsers); |
There was a problem hiding this comment.
Just a nit in this component, so many local states can be better handled with a useReducer or even use react context, but lets do that in an upcoming issue.
… naming Introduce a multi-step AssignRoleWizard with DefineApplicationScopeStep, SelectUsersAndRoleStep, and HighlightedUsersInput components. Singularize roles-permissions subfolders (courses → course, libraries → library) for naming consistency. Update hooks, API, constants, and related tests.
ed75ec9 to
48da926
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Thank for the test instructions, everything works exactly as described!






Caution
This PR requires the validation endpoint introduced in the PR openedx/openedx-authz#245
This PR introduces Step 1 of the Role Assignment Wizard, replacing the previous modal-based approach for adding team members.
What's included:
/assign-roleroute with a two-stepSteppercomponent (AssignRoleWizardPage + AssignRoleWizard)HighlightedUsersInput, a custom textarea with a synchronized overlay layerPOST /api/authz/v1/users/validateendpoint before advancing to Step 2authz-module/constants.tsand re-exported where neededLibrariesTeamManagernow navigates to the wizard instead of opening a modalNot in scope for this PR: Step 2 implementation, course role assignment.
Additional Information
Resolves #91
Screenshots
How to test
1. Open the wizard from the home page
/authz./authz/assign-roleand the wizard Step 1 is shown.2. Open the wizard pre-filled from the user audit view
/authz/libraries/<lib-id>)./authz/assign-rolewith the users input empty andfrom=pointing back to the team page.editaction to open their audit page (/authz/libraries/<lib-id>/<username>).3. Role selection
4. Users input — invalid user
5. Advance to Step 2
6. Cancel and breadcrumb return to the originating view
/authz, click Cancel → confirm you land on/authz./authz.AssignRoleWizard — Step 1 Test Coverage
Cancel returns to the previous viewopens with the user pre-populated when provided via initialUsersshows only course roles when user only has manage_course_team permissionshows only library roles when user only has manage_library_team permissionshows both course and library roles when user has both permissionsselecting a different role replaces the previous selectionblocks progression and highlights the unknown useradvances to Step 2 when all users are validshows a network error and blocks progression when the validation call failsclears the invalid user highlights when the input is edited